Repository navigation
chore(security): upgrade @actions/core, devDependencies, and default CLI to v0.17.1 - #27
Conversation
aatarasoff
left a comment
There was a problem hiding this comment.
Code Review: Security & Reliability Focus
Context: Node.js GitHub Action for installing pbuf CLI (via remote script) and optionally running schema drift checks with sticky PR comments. High-concurrency CI environments, public-facing in the sense of running on arbitrary repos with user-provided tokens/versions.
Findings
-
[Severity: High] — Security & Vulnerabilities
- Location:
src/main.js—run()function, theexec.exec('/bin/bash', ['-lc',curl -fsSL ${INSTALL_SCRIPT_URL} | sh ...])blocks (approx. lines for version install). - Problem: The action downloads and pipes a remote install script (
https://raw.githubusercontent.com/pbufio/pbuf-registry/main/scripts/install.sh) directly intoshwithout any integrity verification (checksum, signature, or commit pin). If the GitHub repo or themainbranch is compromised (or a malicious PR is merged), every workflow using this action becomes a supply-chain attack vector. The version is passed safely as a positional arg for non-latest cases (good), but the script itself is unsigned. This is a classiccurl | shrisk in CI. - Fix: Prefer downloading a pinned release binary directly from GitHub Releases (with optional SHA256 verification), or pin the install script to a specific commit SHA and verify a checksum. Example sketch:
// Prefer direct binary download + checksum for known versions const RELEASE_URL = `https://github.com/pbufio/pbuf-registry/releases/download/${version}/pbuf_${version.slice(1)}_linux_amd64.tar.gz`; // Download, verify sha256 against a known manifest, extract, and chmod +x. // Fall back to script only with explicit user opt-in and clear warning.
- Location:
-
[Severity: Medium] — Security & Vulnerabilities
- Location:
src/main.js—run()→exec.exec('pbuf', ['auth', pbufToken]). - Problem: The registry token is passed as a command-line argument. On Linux this is visible in
/proc/<pid>/cmdlineand process listings (ps, audit logs, container introspection). Combined withcore.setSecret(which only masks logs), it still leaks to the process table for the duration of the auth call. - Fix: Prefer an environment variable or stdin if the CLI supports it. If not:
Ensure the CLI reads from
await exec.exec('pbuf', ['auth'], { env: { ...process.env, PBUF_TOKEN: pbufToken } }); // Or document that the CLI must be updated to read from env/stdin.
PBUF_TOKENor a file.
- Location:
-
[Severity: Medium] — Correctness & Edge Cases / Architecture
- Location: PR title/body vs.
action.yml. - Problem: Title and summary claim "default CLI to v0.17.1", but
action.ymlstill defaults to'v0.17.0'and no source change updates it. Current registry releases appear to be around v0.16.x. This creates a documentation/code mismatch and may confuse consumers. - Fix: Either update
action.ymldefault (and README) to the intended version after confirming it exists and is stable, or correct the PR title/body to reflect only the npm dependency upgrades.
- Location: PR title/body vs.
Positive Notes
- Version regex (
VERSION_RE) properly restricts input before shell use. crypto.randomUUID()replacement for the olduuidpackage in@actions/coreis a solid security improvement (avoids the old uuid advisory).- Proper secret masking, input validation for
fail_on, repository regex, fetch timeouts, and try/catch around the main path. - Bundle size reduction and zero production vulns are good outcomes.
Go / No-Go Verdict
Conditional Go. The dependency upgrades themselves look clean and the rebuilt dist/index.js is consistent with the new @actions/core. No new critical regressions were introduced in the source (which is unchanged). However, the pre-existing curl | sh supply-chain risk is High severity for a setup action used in CI and should be addressed before broad reliance, or at least clearly documented with a mitigation path. The title/default mismatch should be fixed for accuracy. Safe to merge for the security dep bumps once the documentation inconsistency is corrected; schedule a follow-up for the install method.
Review performed as Principal Software Engineer / Security Researcher. Source logic reviewed; dist changes are the expected ncc rebuild.
Summary
@actions/core(^1.11.1),@babel/core,@babel/preset-env,@babel/eslint-parser,eslint,eslint-plugin-jest, andprettier(superseding Bump @babel/preset-env from 7.23.3 to 7.23.9 #18, Bump @babel/core from 7.23.3 to 7.23.9 #19, Bump prettier from 3.1.0 to 3.2.5 #20, Bump eslint-plugin-jest from 27.6.0 to 27.9.0 #21, Bump eslint from 8.54.0 to 8.57.0 #23).npm audit/ Dependabot advisories (uuid,js-yaml,brace-expansion,braces,browserslist,cross-spawn,flatted,minimatch,picomatch,@babel/*), leaving 0 production vulnerabilities (npm audit --omit=dev) and shrinkingdist/index.jsfrom 151 kB to 140 kB.